Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Turn StrategyParameters.NodeResourceUtilizationThresholds field into a pointer #240

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Turn StrategyParameters.NodeResourceUtilizationThresholds field into a pointer #240

merged 1 commit into from
Apr 28, 2020

Conversation

ingvagabund
Copy link
Contributor

The field is intended to be omitempty when not set. Without a pointer the strategy
serialized into json string looks like:

strategies:
  LowNodeUtilization:
    enabled: true
    params:
      nodeResourceUtilizationThresholds:
        numberOfNodes: 1
        targetThresholds:
          cpu: 50
          memory: 50
          pods: 20
        thresholds:
          cpu: 50
          memory: 50
          pods: 20
  RemoveDuplicates:
    enabled: true
    params:
      nodeResourceUtilizationThresholds: {}
  RemovePodsViolatingInterPodAntiAffinity:
    enabled: true
    params:
      nodeResourceUtilizationThresholds: {}
  RemovePodsViolatingNodeAffinity:
    enabled: true
    params:
      nodeAffinityType:
      - requiredDuringSchedulingIgnoredDuringExecution
      nodeResourceUtilizationThresholds: {}
  RemovePodsViolatingNodeTaints:
    enabled: true
    params:
      nodeResourceUtilizationThresholds: {}

It's preferred to have the following json string instead:

strategies:
  LowNodeUtilization:
    enabled: true
    params:
      nodeResourceUtilizationThresholds:
        numberOfNodes: 1
        targetThresholds:
          cpu: 50
          memory: 50
          pods: 20
        thresholds:
          cpu: 50
          memory: 50
          pods: 20
  RemoveDuplicates:
    enabled: true
  RemovePodsViolatingInterPodAntiAffinity:
    enabled: true
  RemovePodsViolatingNodeAffinity:
    enabled: true
    params:
      nodeAffinityType:
      - requiredDuringSchedulingIgnoredDuringExecution
  RemovePodsViolatingNodeTaints:
    enabled: true

This effectively changes the exported data type so any user importing StrategyParameters data type and setting/reading NodeResourceUtilizationThresholds field will need to adjust affected code.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2020
damemi
damemi approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
@seanmalloy any chance this change makes it into v0.10.0? Or are we past that point?

go.mod Outdated
@@ -5,6 +5,7 @@ go 1.13
require (
github.com/spf13/cobra v0.0.5
github.com/spf13/pflag v1.0.5
gopkg.in/yaml.v2 v2.2.8 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea where this came from? says it's indirect but just curious which of these changes might have brought it in

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know the answer to this question. I did compile this code using Go 1.13.6 and can confirm that the go.mod and go.sum files are clean. I also verified go mod tidy and it is good.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2020
@seanmalloy
Copy link
Member

@damemi it is too late for v0.10.0. The tag has already been created https://github.com/kubernetes-sigs/descheduler/releases/tag/v0.10.0.

@seanmalloy
Copy link
Member

/assign @aveshagarwal @ravisantoshgudimetla

@seanmalloy
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 21, 2020
@seanmalloy
Copy link
Member

@aveshagarwal and @ravisantoshgudimetla please take a look at this when you have some time.

@ingvagabund ingvagabund force-pushed the turn-strategy-params-NodeResourceUtilizationThresholds-field-into-pointer branch from 0a3e2ac to e1616a3 Compare April 28, 2020 13:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2020
@ingvagabund
Turn StrategyParameters.NodeResourceUtilizationThresholds field into …
991eddb 
…a pointer

The field is intended to be omitempty when not set. Without a pointer the strategy
serialized into json string looks like:

```json
strategies:
  LowNodeUtilization:
    enabled: true
    params:
      nodeResourceUtilizationThresholds:
        numberOfNodes: 1
        targetThresholds:
          cpu: 50
          memory: 50
          pods: 20
        thresholds:
          cpu: 50
          memory: 50
          pods: 20
  RemoveDuplicates:
    enabled: true
    params:
      nodeResourceUtilizationThresholds: {}
  RemovePodsViolatingInterPodAntiAffinity:
    enabled: true
    params:
      nodeResourceUtilizationThresholds: {}
  RemovePodsViolatingNodeAffinity:
    enabled: true
    params:
      nodeAffinityType:
      - requiredDuringSchedulingIgnoredDuringExecution
      nodeResourceUtilizationThresholds: {}
  RemovePodsViolatingNodeTaints:
    enabled: true
    params:
      nodeResourceUtilizationThresholds: {}
```

It's preferred to have the following json string instead:
```
strategies:
  LowNodeUtilization:
    enabled: true
    params:
      nodeResourceUtilizationThresholds:
        numberOfNodes: 1
        targetThresholds:
          cpu: 50
          memory: 50
          pods: 20
        thresholds:
          cpu: 50
          memory: 50
          pods: 20
  RemoveDuplicates:
    enabled: true
  RemovePodsViolatingInterPodAntiAffinity:
    enabled: true
  RemovePodsViolatingNodeAffinity:
    enabled: true
    params:
      nodeAffinityType:
      - requiredDuringSchedulingIgnoredDuringExecution
  RemovePodsViolatingNodeTaints:
    enabled: true
```
@ingvagabund ingvagabund force-pushed the turn-strategy-params-NodeResourceUtilizationThresholds-field-into-pointer branch from e1616a3 to 991eddb Compare April 28, 2020 13:22
damemi
damemi approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2020
@ravisantoshgudimetla
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, ravisantoshgudimetla

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ravisantoshgudimetla]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit 149085f into kubernetes-sigs:master Apr 28, 2020
@ingvagabund ingvagabund deleted the turn-strategy-params-NodeResourceUtilizationThresholds-field-into-pointer branch April 28, 2020 13:59
@ingvagabund ingvagabund mentioned this pull request Apr 28, 2020
Merged
@damemi
Copy link
Contributor

damemi commented May 6, 2020

To resurrect this, do you think we should do the same with the PodsHavingTooManyRestarts strategy params? I think we'll hit the same problem there too now

@ingvagabund
Copy link
Contributor Author

ingvagabund commented May 6, 2020 •
edited
Loading

To resurrect this, do you think we should do the same with the PodsHavingTooManyRestarts strategy params? I think we'll hit the same problem there too now

Yeah, given the Strategy data type is shared among all strategies, more pointers, the better!!!

@damemi damemi mentioned this pull request May 6, 2020
Merged
@seanmalloy seanmalloy mentioned this pull request May 20, 2020
Closed
4 tasks
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
Merge pull request kubernetes-sigs#240 from ingvagabund/turn-strategy…
ca2bdcf 
…-params-NodeResourceUtilizationThresholds-field-into-pointer

Turn StrategyParameters.NodeResourceUtilizationThresholds field into a pointer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damemi damemi damemi approved these changes

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

Assignees

@damemi damemi

@aveshagarwal aveshagarwal

@ravisantoshgudimetla ravisantoshgudimetla

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ingvagabund @seanmalloy @ravisantoshgudimetla @k8s-ci-robot @damemi @aveshagarwal